Warn when doing a PEP 517 build in presence of --global-option #9774

sbidoul · 2021-04-04T12:47:11Z

Warn when doing a PEP 517 build in presence of --global-option
Warn instead of erroring out when doing a PEP 517 build in presence of --build-option

--build-option and --global-option are not supported by PEP 517.
Before, --global-option was silently ignored and --build-option caused an error.

We now warn if any of the two is present, in order to be more explicit about ignored options, and to avoid any backward compatibility issues when we start passing --build-option in the install command.

This is a first step towards #9769. I'll see if I can split #9422 in small PRs to facilitate review.

We warn instead of erroring out when --build-option is present when doing a PEP 517 build. There is no strong reason to error out, and this will avoid backward compatibility issues when we support build options in requirement files and installation.

So these are not ignored silently.

pradyunsg · 2021-04-04T14:42:22Z

Can we keep the error for --build-option? I'm not sure we gain anything by relaxing behavior there.

sbidoul · 2021-04-04T15:17:46Z

@pradyunsg we gain symmetry in the behaviour of --global-option and --build-option. I like symmetry :)
I don't think anyone would actually rely on this erroring out ?
Also, when the install command will accept --build-option it might be more flexible to not fail abruptly in this case (say, when someone installs a mix of legacy and PEP 517 with --build-option enabled).

pradyunsg · 2021-04-04T21:13:33Z

I don't think anyone would actually rely on this erroring out?

No, but I guess they will rely on it not erroring out in the future; especially since the warning isn't stating that we'd disable it in the future or something to discourage it. If we're relaxing some behaviour, we should probably have a good reason for it; because getting stricter is much harder later.

Anyway, I don't feel strongly about this and I don't have enough context to understand how this change helps the broader bigger PR. Just to be overly verbose, I'm not blocking this or whatever; just trying to understand what is happening. :)

sbidoul · 2021-04-05T15:26:57Z

@pradyunsg thanks for the feedback.

I'm not too worried about getting stricter in the future, because we ignore the option so it's a no-op.

TBH the scenarios in which the error would actually be an issue might be a bit far fetched (e.g. when someone installs a mix of legacy and PEP 517 with --build-option enabled). But I prefer being flexible with users here. And as a nice bonus, the code is cleaner with the same processing for --global-option and --build-option in this area, and build_wheel_pep517() that needs not be concerned anymore with an option that has nothing to do with it.

pradyunsg · 2021-04-05T20:01:47Z

TBH the scenarios in which the error would actually be an issue might be a bit far fetched (e.g. when someone installs a mix of legacy and PEP 517 with --build-option enabled).

Well, maybe. We're switching from error to fairly-silent-ignore... I guess it triggers some tripwire in my brain. For the future me reading this, these options can be set on a per-requirement basis too.

I guess my question is what's the long term plan here? Do we plan to allow these in some form for PEP 517 builds? If not, I guess prefer we change the calls to be calls to deprecated for 21.3 instead of logger.warn.

pradyunsg · 2021-04-05T20:02:11Z

(oh yea, and the code is definitely cleaner like this)

sbidoul · 2021-04-06T17:44:47Z

Do we plan to allow these in some form for PEP 517 builds?

I don't think so. For PEP 517 it will be some sort of config-settings (#5771), so a new option (unless config-settings end up never being needed by anyone, we'll see with the feedback we get on #9769).

If not, I guess prefer we change the calls to be calls to deprecated for 21.3 instead of logger.warn.

I suppose we'll deprecate --build-options and --global-option, at the same time as we deprecate all calls to setup.py bdist_wheel. But that is premature: we first need to deprecate setup.py install and we do that by using setup.py bdist_wheel which is softer transition than enforcing PEP 517.

uranusjr · 2021-04-07T12:26:38Z

It kind-of-sort-of makes sense to re-use --build-option for PEP 517 config_settings, but it’s probably easier in more than one way if we invent a new option for it. The deprecation of --build-option and --global-option will also help us identify if we really need config_settings at all, i.e. what people are using --build-option and --global-option right now for, minus the location options (e.g. --prefix).

sbidoul · 2021-04-07T13:11:40Z

A deprecation without a replacement is not ideal, though. What we could do is add an info or warning level message when build_wheel_legacy is called with build_options or global_options. This message could direct users to a GitHub issue asking them to describe their use cases.

Bumps [pip](https://github.com/pypa/pip) from 21.0.1 to 21.1. <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/pypa/pip/blob/main/NEWS.rst">pip's changelog</a>.</em></p> <blockquote> <h1>21.1 (2021-04-24)</h1> <h2>Process</h2> <ul> <li>Start installation scheme migration from <code>distutils</code> to <code>sysconfig</code>. A warning is implemented to detect differences between the two implementations to encourage user reports, so we can avoid breakages before they happen.</li> </ul> <h2>Features</h2> <ul> <li>Add the ability for the new resolver to process URL constraints. (<code>[#8253](pypa/pip#8253) <https://github.com/pypa/pip/issues/8253></code>_)</li> <li>Add a feature <code>--use-feature=in-tree-build</code> to build local projects in-place when installing. This is expected to become the default behavior in pip 21.3; see <code>Installing from local packages <https://pip.pypa.io/en/stable/user_guide/#installing-from-local-packages></code>_ for more information. (<code>[#9091](pypa/pip#9091) <https://github.com/pypa/pip/issues/9091></code>_)</li> <li>Bring back the "(from versions: ...)" message, that was shown on resolution failures. (<code>[#9139](pypa/pip#9139) <https://github.com/pypa/pip/issues/9139></code>_)</li> <li>Add support for editable installs for project with only setup.cfg files. (<code>[#9547](pypa/pip#9547) <https://github.com/pypa/pip/issues/9547></code>_)</li> <li>Improve performance when picking the best file from indexes during <code>pip install</code>. (<code>[#9748](pypa/pip#9748) <https://github.com/pypa/pip/issues/9748></code>_)</li> <li>Warn instead of erroring out when doing a PEP 517 build in presence of <code>--build-option</code>. Warn when doing a PEP 517 build in presence of <code>--global-option</code>. (<code>[#9774](pypa/pip#9774) <https://github.com/pypa/pip/issues/9774></code>_)</li> </ul> <h2>Bug Fixes</h2> <ul> <li>Fixed <code>--target</code> to work with <code>--editable</code> installs. (<code>[#4390](pypa/pip#4390) <https://github.com/pypa/pip/issues/4390></code>_)</li> <li>Add a warning, discouraging the usage of pip as root, outside a virtual environment. (<code>[#6409](pypa/pip#6409) <https://github.com/pypa/pip/issues/6409></code>_)</li> <li>Ignore <code>.dist-info</code> directories if the stem is not a valid Python distribution name, so they don't show up in e.g. <code>pip freeze</code>. (<code>[#7269](pypa/pip#7269) <https://github.com/pypa/pip/issues/7269></code>_)</li> <li>Only query the keyring for URLs that actually trigger error 401. This prevents an unnecessary keyring unlock prompt on every pip install invocation (even with default index URL which is not password protected). (<code>[#8090](pypa/pip#8090) <https://github.com/pypa/pip/issues/8090></code>_)</li> <li>Prevent packages already-installed alongside with pip to be injected into an isolated build environment during build-time dependency population. (<code>[#8214](pypa/pip#8214) <https://github.com/pypa/pip/issues/8214></code>_)</li> <li>Fix <code>pip freeze</code> permission denied error in order to display an understandable error message and offer solutions. (<code>[#8418](pypa/pip#8418) <https://github.com/pypa/pip/issues/8418></code>_)</li> <li>Correctly uninstall script files (from setuptools' <code>scripts</code> argument), when installed with <code>--user</code>. (<code>[#8733](pypa/pip#8733) <https://github.com/pypa/pip/issues/8733></code>_)</li> <li>New resolver: When a requirement is requested both via a direct URL (<code>req @ URL</code>) and via version specifier with extras (<code>req[extra]</code>), the resolver will now be able to use the URL to correctly resolve the requirement with extras. (<code>[#8785](pypa/pip#8785) <https://github.com/pypa/pip/issues/8785></code>_)</li> <li>New resolver: Show relevant entries from user-supplied constraint files in the error message to improve debuggability. (<code>[#9300](pypa/pip#9300) <https://github.com/pypa/pip/issues/9300></code>_)</li> <li>Avoid parsing version to make the version check more robust against lousily debundled downstream distributions. (<code>[#9348](pypa/pip#9348) <https://github.com/pypa/pip/issues/9348></code>_)</li> <li><code>--user</code> is no longer suggested incorrectly when pip fails with a permission error in a virtual environment. (<code>[#9409](pypa/pip#9409) <https://github.com/pypa/pip/issues/9409></code>_)</li> <li>Fix incorrect reporting on <code>Requires-Python</code> conflicts. (<code>[#9541](pypa/pip#9541) <https://github.com/pypa/pip/issues/9541></code>_)</li> </ul>  </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/pypa/pip/commit/2b2a268d25963727c2a1c805de8f0246b9cd63f6"><code>2b2a268</code></a> Bump for release</li> <li><a href="https://github.com/pypa/pip/commit/ea761a6575f37b90cf89035ee8be3808cf872184"><code>ea761a6</code></a> Update AUTHORS.txt</li> <li><a href="https://github.com/pypa/pip/commit/2edd3fdf2af2f09dce5085ef0eb54684b4f9bc04"><code>2edd3fd</code></a> Postpone a deprecation to 21.2</li> <li><a href="https://github.com/pypa/pip/commit/3cccfbf169bd35133ee25d2543659b9c1e262f8c"><code>3cccfbf</code></a> Rename mislabeled news fragment</li> <li><a href="https://github.com/pypa/pip/commit/21cd124b5d40b510295c201b9152a65ac3337a37"><code>21cd124</code></a> Fix NEWS.rst placeholder position</li> <li><a href="https://github.com/pypa/pip/commit/e46bdda9711392fec0c45c1175bae6db847cb30b"><code>e46bdda</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/pypa/pip/issues/9827">#9827</a> from pradyunsg/fix-git-improper-tag-handling</li> <li><a href="https://github.com/pypa/pip/commit/0e4938d269815a5bf1dd8c16e851cb1199fc5317"><code>0e4938d</code></a> 📰</li> <li><a href="https://github.com/pypa/pip/commit/ca832b2836e0bffa7cf95589acdcd71230f5834e"><code>ca832b2</code></a> Don't split git references on unicode separators</li> <li><a href="https://github.com/pypa/pip/commit/1320bac4ff80d76b8fba2c8b4b4614a40fb9c6c3"><code>1320bac</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/pypa/pip/issues/9814">#9814</a> from pradyunsg/revamp-ci-apr-2021-v2</li> <li><a href="https://github.com/pypa/pip/commit/e9cc23ffd97cb6d66d32dc3ec27cf832524bb33d"><code>e9cc23f</code></a> Skip checks on PRs only</li> <li>Additional commits viewable in <a href="https://github.com/pypa/pip/compare/21.0.1...21.1">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=pip&package-manager=pip&previous-version=21.0.1&new-version=21.1)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually </details>

Bumps [pip](https://github.com/pypa/pip) from 21.0.1 to 21.1.1. <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/pypa/pip/blob/main/NEWS.rst">pip's changelog</a>.</em></p> <blockquote> <h1>21.1.1 (2021-04-30)</h1> <h2>Deprecations and Removals</h2> <ul> <li>Temporarily set the new "Value for ... does not match" location warnings level to <em>DEBUG</em>, to hide them from casual users. This prepares pip 21.1 for CPython inclusion, while pip maintainers digest the first intake of location mismatch issues for the <code>distutils</code>-<code>sysconfig</code> transition. (<code>[#9912](pypa/pip#9912) <https://github.com/pypa/pip/issues/9912></code>_)</li> </ul> <h2>Bug Fixes</h2> <ul> <li>This change fixes a bug on Python `_)</li> <li>Fix compatibility between distutils and sysconfig when the project name is unknown outside of a virtual environment. (<code>[#9838](pypa/pip#9838) <https://github.com/pypa/pip/issues/9838></code>_)</li> <li>Fix Python 3.6 compatibility when a PEP 517 build requirement itself needs to be built in an isolated environment. (<code>[#9878](pypa/pip#9878) <https://github.com/pypa/pip/issues/9878></code>_)</li> </ul> <h1>21.1 (2021-04-24)</h1> <h2>Process</h2> <ul> <li>Start installation scheme migration from <code>distutils</code> to <code>sysconfig</code>. A warning is implemented to detect differences between the two implementations to encourage user reports, so we can avoid breakages before they happen.</li> </ul> <h2>Features</h2> <ul> <li>Add the ability for the new resolver to process URL constraints. (<code>[#8253](pypa/pip#8253) <https://github.com/pypa/pip/issues/8253></code>_)</li> <li>Add a feature <code>--use-feature=in-tree-build</code> to build local projects in-place when installing. This is expected to become the default behavior in pip 21.3; see <code>Installing from local packages <https://pip.pypa.io/en/stable/user_guide/#installing-from-local-packages></code>_ for more information. (<code>[#9091](pypa/pip#9091) <https://github.com/pypa/pip/issues/9091></code>_)</li> <li>Bring back the "(from versions: ...)" message, that was shown on resolution failures. (<code>[#9139](pypa/pip#9139) <https://github.com/pypa/pip/issues/9139></code>_)</li> <li>Add support for editable installs for project with only setup.cfg files. (<code>[#9547](pypa/pip#9547) <https://github.com/pypa/pip/issues/9547></code>_)</li> <li>Improve performance when picking the best file from indexes during <code>pip install</code>. (<code>[#9748](pypa/pip#9748) <https://github.com/pypa/pip/issues/9748></code>_)</li> <li>Warn instead of erroring out when doing a PEP 517 build in presence of <code>--build-option</code>. Warn when doing a PEP 517 build in presence of <code>--global-option</code>. (<code>[#9774](pypa/pip#9774) <https://github.com/pypa/pip/issues/9774></code>_)</li> </ul> <h2>Bug Fixes</h2> <ul> <li>Fixed <code>--target</code> to work with <code>--editable</code> installs. (<code>[#4390](pypa/pip#4390) <https://github.com/pypa/pip/issues/4390></code>_)</li> <li>Add a warning, discouraging the usage of pip as root, outside a virtual environment. (<code>[#6409](pypa/pip#6409) <https://github.com/pypa/pip/issues/6409></code>_)</li> </ul>  </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/pypa/pip/commit/c53d88c4c374523425f4db6bef949090764465c0"><code>c53d88c</code></a> Bump for release</li> <li><a href="https://github.com/pypa/pip/commit/4417e7f4bef2b2c70767c1dbfe72f82cc6b7b83f"><code>4417e7f</code></a> Update AUTHORS.txt</li> <li><a href="https://github.com/pypa/pip/commit/0c29bfe48e650c5a428b77eba4af7f067c019cc8"><code>0c29bfe</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/pypa/pip/issues/9912">#9912</a> from uranusjr/sysconfig-remove-warning-for-python-re...</li> <li><a href="https://github.com/pypa/pip/commit/f56ec327b92ebe836e63e07cb2449a20e09dec38"><code>f56ec32</code></a> Make location mismatch messages DEBUG level</li> <li><a href="https://github.com/pypa/pip/commit/999b121402302a00b235a0d443f5661b69d6fd60"><code>999b121</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/pypa/pip/issues/9883">#9883</a> from uranusjr/isolated-pip-py36-compat</li> <li><a href="https://github.com/pypa/pip/commit/f88420319db450aefbed1500f04e31be46874aaf"><code>f884203</code></a> Fallback to self-invoke via directory on 3.6</li> <li><a href="https://github.com/pypa/pip/commit/7a77484a492c8f1e1f5ef24eaf71a43df9ea47eb"><code>7a77484</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/pypa/pip/issues/9835">#9835</a> from jamescurtin/9831-bugfix</li> <li><a href="https://github.com/pypa/pip/commit/914bcc3dba88179f4e88ce90b63660474ba795cd"><code>914bcc3</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/pypa/pip/issues/9838">#9838</a> from uranusjr/sysconfig-header-with-none-project</li> <li><a href="https://github.com/pypa/pip/commit/2a009a0b8a5d8d03117897f0f11f9c1dcf2a4b5a"><code>2a009a0</code></a> Better explanatory comment</li> <li><a href="https://github.com/pypa/pip/commit/e7b1722efeaf4ff403142847ce1b52caedd5efcd"><code>e7b1722</code></a> Set dist_name to UNKNOWN when empty outside venv</li> <li>Additional commits viewable in <a href="https://github.com/pypa/pip/compare/21.0.1...21.1.1">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=pip&package-manager=pip&previous-version=21.0.1&new-version=21.1.1)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually </details>

sbidoul force-pushed the warn-on-pep517-build-global-option branch from fab96c6 to 46e247f Compare April 4, 2021 12:48

uranusjr approved these changes Apr 4, 2021

View reviewed changes

sbidoul added 2 commits April 4, 2021 16:08

PEP 517 build in presence of --global-option emits a warning

7c6ee48

So these are not ignored silently.

sbidoul force-pushed the warn-on-pep517-build-global-option branch from 46e247f to 7c6ee48 Compare April 4, 2021 14:08

xavfernandez approved these changes Apr 14, 2021

View reviewed changes

uranusjr added this to the 21.1 milestone Apr 14, 2021

uranusjr merged commit d4c32a2 into pypa:main Apr 14, 2021

sbidoul deleted the warn-on-pep517-build-global-option branch April 15, 2021 06:50

sbidoul mentioned this pull request Apr 15, 2021

Add warning when build_wheel_legacy is called with build_options or global_options #9807

Open

github-actions bot locked as resolved and limited conversation to collaborators Sep 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn when doing a PEP 517 build in presence of --global-option #9774

Warn when doing a PEP 517 build in presence of --global-option #9774

sbidoul commented Apr 4, 2021 •

edited

Loading

pradyunsg commented Apr 4, 2021

sbidoul commented Apr 4, 2021

pradyunsg commented Apr 4, 2021

sbidoul commented Apr 5, 2021 •

edited

Loading

pradyunsg commented Apr 5, 2021 •

edited

Loading

pradyunsg commented Apr 5, 2021

sbidoul commented Apr 6, 2021 •

edited

Loading

uranusjr commented Apr 7, 2021

sbidoul commented Apr 7, 2021

Warn when doing a PEP 517 build in presence of --global-option #9774

Warn when doing a PEP 517 build in presence of --global-option #9774

Conversation

sbidoul commented Apr 4, 2021 • edited Loading

pradyunsg commented Apr 4, 2021

sbidoul commented Apr 4, 2021

pradyunsg commented Apr 4, 2021

sbidoul commented Apr 5, 2021 • edited Loading

pradyunsg commented Apr 5, 2021 • edited Loading

pradyunsg commented Apr 5, 2021

sbidoul commented Apr 6, 2021 • edited Loading

uranusjr commented Apr 7, 2021

sbidoul commented Apr 7, 2021

sbidoul commented Apr 4, 2021 •

edited

Loading

sbidoul commented Apr 5, 2021 •

edited

Loading

pradyunsg commented Apr 5, 2021 •

edited

Loading

sbidoul commented Apr 6, 2021 •

edited

Loading